Skip to content

fix: enforce the table allow-list on request-group query members - #1098

Merged
babltiga merged 2 commits into
mainfrom
claude/bold-khorana-2954dd
Sep 25, 2026
Merged

babltiga merged 2 commits into
mainfrom
claude/bold-khorana-2954dd

Conversation

@babltiga

Copy link
Copy Markdown
Contributor

What

DefaultRequestGroupService.validatePermission checked a QUERY member's capability and its denied columns (#935). It never checked the permission's allowed_schemas / allowed_tables. Standard submission (DatasourcePermissionVerifier) and standalone break-glass (DefaultBreakGlassService.verifyBreakGlassPermission) both check them. As a result, a request-group member could reference a table outside the submitter's allow-list.

Change

  • verifyDeniedColumns is replaced by verifyTableAndColumnScope. It checks every referenced table against the allow-list using core.api.AllowedTables (normalize + coveringEntry), then runs the existing denied-column check. The rules match DatasourcePermissionChecker.rejectedTables:
    • both lists empty means no restriction;
    • a schema entry covers any table qualified with that schema;
    • a bare table entry covers only an unqualified reference.
  • A miss throws RequestGroupPermissionException, which the web layer returns as a localized 403. The group stays DRAFT.
  • The query is parsed once, and only when the permission has an allow-list or denied columns.
  • Break-glass groups are covered too. Standalone break-glass enforces the allow-list, and break-glass skips approval, not data protection.
  • The QUERY_ADMIN path is unchanged. It still skips the per-datasource gate, as on standard submission.

Tests

Six new cases in DefaultRequestGroupServiceCrudTest:

  • a table outside the allow-list is rejected;
  • tables covered by a table entry or a schema entry are accepted;
  • a bare entry covers orders but not public.orders;
  • empty lists are unrestricted and the query isn't parsed;
  • break-glass rejects a table outside the allow-list and never executes;
  • admins are not bound by the allow-list.

DefaultRequestGroupService*Test passes locally (27 tests). The full mvn verify has not been run.

Docs

  • docs/05-backend.md, request-groups "Build-time permission validation": the table-scope rules.
  • docs/07-security.md, permission flow: the allow-list step also binds request-group members.

Reviewer notes

  • core: table deny-lists alongside allow-lists #939 (table deny-lists) isn't merged yet. When it lands, its deny check belongs in verifyTableAndColumnScope.
  • Out of scope, worth a follow-up:
    • Group break-glass members don't check the capability their query type needs; standalone break-glass does.
    • The group path doesn't check expiresAt itself. I didn't check whether findFor already filters out expired grants.

DefaultRequestGroupService.validatePermission checked a QUERY member's capability and denied columns but never allowed_schemas/allowed_tables, so a group member could reference a table outside the submitter's allow-list. Check it through core.api.AllowedTables with DatasourcePermissionChecker.rejectedTables semantics, on the break-glass path too (standalone break-glass enforces it).
…954dd

# Conflicts:
#	backend/src/main/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupService.java
@github-actions

Copy link
Copy Markdown
Contributor

Backend Test Results

10 393 tests  +6   10 393 ✅ +6   12m 19s ⏱️ +46s
 1 147 suites ±0        0 💤 ±0 
 1 147 files   ±0        0 ❌ ±0 

Results for commit eeef431. ± Comparison against base commit 9fc3865.

@github-actions

Copy link
Copy Markdown
Contributor

Backend Code Coverage

Overall Project 94.77% 🍏
Files changed 100% 🍏

File Coverage
DefaultRequestGroupService.java 82.41% 🍏

@babltiga
babltiga merged commit f12670a into main Sep 25, 2026
34 checks passed
@babltiga
babltiga deleted the claude/bold-khorana-2954dd branch September 25, 2026 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant